Skip to content

Fix HPACK dynamic table desync#2628

Draft
const-t wants to merge 17 commits into
masterfrom
kt-fix-hpack-desync
Draft

Fix HPACK dynamic table desync#2628
const-t wants to merge 17 commits into
masterfrom
kt-fix-hpack-desync

Conversation

@const-t

@const-t const-t commented Mar 11, 2026

Copy link
Copy Markdown
Contributor

No description provided.

Comment thread fw/http_frame.c Outdated
@const-t const-t changed the title Fix HAPCK dynamic table desync Fix HPACK dynamic table desync Mar 11, 2026
@const-t const-t force-pushed the kt-fix-hpack-desync branch 4 times, most recently from f48b3f2 to c7e24c8 Compare March 17, 2026 14:54
@const-t

const-t commented Mar 17, 2026

Copy link
Copy Markdown
Contributor Author

@krizhanovsky @EvgeniiMekhanik the commit refactor: Move skb related functions to ss_skb.h is optional and can be dropped. It was introduced as part of the commit where I reworked trailers encoding, however we decided to not use HPACK dynamic table for trailers and I dropped that patch, but this commit with moving functions left in the PR.

@const-t const-t marked this pull request as ready for review March 17, 2026 15:18
@const-t const-t requested review from EvgeniiMekhanik and krizhanovsky and removed request for krizhanovsky March 18, 2026 14:34
@const-t const-t force-pushed the kt-fix-hpack-desync branch from c7e24c8 to 9cd2132 Compare March 30, 2026 12:47
Comment thread fw/http_stream.c Outdated
do { \
if ((ctx->cur_##op##_headers \
&& (type != HTTP2_CONTINUATION && type != HTTP2_RST_STREAM)) \
&& ((type == HTTP2_HEADERS && !is_send) || \

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It seems that it is wrong. A HEADERS frame without the END_HEADERS flag set MUST be followed by a CONTINUATION frame for the same stream. A receiver MUST treat the receipt of any other type of frame or a frame on a different stream as a connection error (Section 5.4.1) of type PROTOCOL_ERROR.

It seems that HTTP2_RST_STREAM is also forbidden. So it should be
if ((ctx->cur_##op##_headers and type != HTTP2_CONTINUATION)

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think it's better to check tcp window and do not prepare response. Or ignore tcp windo limit for headers frame
.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Added fix for this as well.

@EvgeniiMekhanik EvgeniiMekhanik left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think we should not prepare response if there is no enough TCP window or ignore tcp window during sending headers frame. Otherwise we need make incorrect change in stream state processing. @krizhanovsky what do you think?

@EvgeniiMekhanik

Copy link
Copy Markdown
Contributor

I also found two problems in
f (frame_length < min_len)
ADJUST_BLOCKED_STREAMS_AND_EXIT(min_len, type);
optimization:

  • we stop making frames if stream http2 window is exceeded. But if we have other streams to make frames we should do it.
  • we stop making frames if http2 connection window is exceeded. But if we have other streams with headers frames
    we should continue make frames for them.

@const-t const-t marked this pull request as draft April 9, 2026 12:46
@const-t const-t force-pushed the kt-fix-hpack-desync branch 3 times, most recently from 723a84e to 2912200 Compare May 1, 2026 10:43
@const-t const-t linked an issue May 1, 2026 that may be closed by this pull request
@const-t const-t force-pushed the kt-fix-hpack-desync branch from 2912200 to f9687bb Compare May 1, 2026 15:38
@const-t const-t marked this pull request as ready for review May 1, 2026 15:38
@const-t

const-t commented May 4, 2026

Copy link
Copy Markdown
Contributor Author

I think we should not prepare response if there is no enough TCP window or ignore tcp window during sending headers frame. Otherwise we need make incorrect change in stream state processing. @krizhanovsky what do you think?

I've updated PR. Added fixes for #2640. But I didn't implement I think we should not prepare response if there is no enough TCP window. From my point of view, if we reached framing function we can prepare headers even if tcp window is not enough, to have prepared headers on the next shot of calling sending function and don't call stream scheduler one more time. However I attached the version where we don't prepare response if there is no enough TCP window in the comment to see the difference.

Actually this patch allows to call tfw_h2_stream_fsm_ignore_err() multiple times for HEADERS frame in the state HTTP2_STREAM_REM_HALF_CLOSED only during sending that doesn't looks like issue, because we only assign ctx->cur_send_headers in this case and not change state of the stream, frames received from the client will not affect this change only sending side.

@const-t const-t requested a review from EvgeniiMekhanik May 4, 2026 16:23
Comment thread fw/hpack.c Outdated
*/
if (tbl->window != new_size && (likely(!tbl->wnd_changed)
|| unlikely(!tbl->window) || new_size < tbl->window))
if (tbl->window != requested_size && (likely(!tbl->wnd_changed)

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I read RFC again and look for nghtt2 realization - in some cases we should apply and send two hpack dynamic changes!
For example
4096 - > 1 -> 100 we should send 1 and 100 (minimum and final)
4096 -> 1 -> 0 -> 2 -> 100 -> 3 we should send 0 and 3 (minimum and final)

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good catch, another one bug. Tempesta must be able to send min and max size. RFC cite:

Multiple updates to the maximum table size can occur between the
transmission of two header blocks. In the case that this size is
changed more than once in this interval, the smallest maximum table
size that occurs in that interval MUST be signaled in a dynamic table
size update. The final maximum size is always signaled, resulting in
at most two dynamic table size updates. This ensures that the
decoder is able to perform eviction based on reductions in dynamic
table size

However maybe it makes sense to implement it in other PR, to keep this PR small.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes also look nghttp2_hd_deflate_hd_bufs in nghttp2

if (deflater->ctx.hd_table_bufsize_max > min_hd_table_bufsize_max) {

      rv = emit_table_size(bufs, min_hd_table_bufsize_max);

      if (rv != 0) {
        goto fail;
      }
    }

    rv = emit_table_size(bufs, deflater->ctx.hd_table_bufsize_max);

@EvgeniiMekhanik

EvgeniiMekhanik commented May 8, 2026

Copy link
Copy Markdown
Contributor

There are some questions about patch:

  1. If we can't send any other frames for other streams after sending HEADERS FRAME without END_STREAM flag for current stream may be it have sense to don't calculate snd_wnd during sending headers at all? In this case we don't need some commits from this patch. (UPDATE. I check h20 implementation they don't send all headers to socket immediately< also we rely on window when we account client mem, so we still should calculate snd_wnd during sending headers, but may be not do it so accuracy).
  2. We should check how another implementations works with RST STREAM receiving. It seems that now be make some changes not according RFC. It looks like we are trying to simply hide the current problem rather than solve it globally. (I mean that we continue to send headers frames after receiving RST STREAM and more over we don't process it now at all!)

Comment thread fw/http_frame.c Outdated
} else if (res == STREAM_FSM_RES_TERM_STREAM) { \
WARN_ON_ONCE(hdr->stream_id != ctx->cur_stream->id); \
return tfw_h2_current_stream_send_rst((ctx), err); \
return tfw_h2_send_rst_stream(ctx, ctx->cur_stream->id,\

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

May we can implement tfw_h2_on_tcp_entail_rst (like tfw_h2_on_tcp_entail_ack). to move stream to closed queue?

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Currently it will be moved to closed queue during making data frames (because of error in tfw_h2_insert_frame_header->tfw_h2_stream_fsm_ignore_err but it is not clear (as for me)

@const-t const-t May 12, 2026

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Currently it will be moved to closed queue during making data frames (because of error in tfw_h2_insert_frame_header->tfw_h2_stream_fsm_ignore_err but it is not clear (as for me)

This is correct for requests for that we will prepare response, however if response will not be prepared we will not add stream to closed and don't clean before connection closing. I will fix this.

@const-t const-t marked this pull request as draft May 14, 2026 09:06
@const-t const-t force-pushed the kt-fix-hpack-desync branch from f9687bb to 0edde4e Compare May 14, 2026 09:13
const-t added 15 commits June 9, 2026 17:08
Advertise the selected dynamic table size if the client's table size is
not equal to Tempesta FW dynamic table size.

Details: when Tempesta receives SETTINGS frame and
SETTINGS_HEADER_TABLE_SIZE in this frame is greater than default
Tempesta's 4096, Tempesta advertise the size that it will be used.
Before this patch Tempesta ignored it when Tempesta's table size is
equal to default 4096 and size advertised by the client is greater than
4096. In other cases Tempesta did send updates.

For reference see: RFC7541 Section 4.2
This patch fixes two issues with HPACK desync. The first one appears
when we encode trailer headers at the moment of encoding regular headers
For example: Tempesta encoded headers and trailers and proceed to
sending the body. During sending the body scheduler few times preemt
the current stream to send headers for another stream, therefore HPACK
dynamic table contains the new headers and indexes used for trailers are
invalid. To fix this we decided to not use HPACK dynamic table for
trailers, it is a good trade-off between complexity and efficiency.

The reason of the second is not sending new size of the dynamic table
in trailer HEADERS frame. When client advertised the new size of the
dynamic table Tempesta must respond with selected size in the first
HEADERS it doesn't matter whether they are trailers or regular headers.

Small refactoring to improve readability
By the standart during sending headers block any others frames are
forbidden except CONTINUATION frame
Before this patch Tempesta stopped to send headers block when
RST_STREAM frame was received or sent, that broke HPACK state. In
this patch we sending all encoded headers to maintain synchronized
HPACK dynamic table.
The case when Tempesta receives obviously broken HEADERS or PRIORITY
frame Tempesta treats it as suspicios and do disconnect instead of
handling this case with RST_STREAM. It looks that it doesn't makes
sense to continue service this connection. Also this removes the attack
vector for RST_STREAM flood.

RFC 9113 5.4.1. Connection Error Handling:
An endpoint can end a connection at any time. In particular, an endpoint
MAY choose to treat a stream error as a connection error.
Do this to ensure that the function calculating frame size accounts for
the dynamic table size, not just the headers, and does not produce
CONTINUATION frame if the data fits within a single HEADERS frame.

The main reason to fix this:
It seems firefox has a bug because of that it can't proccess
CONTINUATION frame for closed stream and resets connection breaking
page loading.
The function must have only one responsibility insert frame headers,
however before this patch it also sent data. In this patch we
extract data sending logic into dedicated state to make code more
understandable and explicit
This patch enables splitting HEADERS and CONTINUATION frames according
to the maximum frame length and sends the resulting frame fragments
as the sending window becomes available. Length of prepared frame stored
in `TfwHttpXmit` struct. This avoids generating a large number of
small CONTINUATION frames, which may be rejected by some
implementations, such as nghttp.
This is a similar optimization to the one used in the previous framing
implementation. However, this version enforces the minimum sending
budget only for the TCP send window, not for the HTTP/2 flow-control
window. This allows to send small DATA frame and proceed to the next
stream if TCP window is available.

In addition, this patch no longer stops re-scheduling when a stream's
flow-control window is exhausted. Instead, it continues searching for
the next stream with available window capacity.
According to the HTTP/2 specification, when two or more
`SETTINGS_HEADER_TABLE_SIZE` values are received between header blocks,
the receiver must send at most two dynamic table updates. These updates
must include the minimum received and applied value and the currently
applied value.

The minimum applied value is needed for the sender of the setting
`SETTINGS_HEADER_TABLE_SIZE` to perform the same eviction in the
decoder’s dynamic table.
@const-t const-t force-pushed the kt-fix-hpack-desync branch from 0edde4e to b462bbf Compare June 12, 2026 20:23
@const-t const-t marked this pull request as ready for review June 12, 2026 20:25
@const-t const-t marked this pull request as draft June 13, 2026 09:34
const-t added 2 commits June 13, 2026 12:39
This patch fixes two issues related to SETTINGS handling. The first is
a kernel panic that occurs when applying a new
`SETTINGS_INITIAL_WINDOW_SIZE`.

When Tempesta receives a new `SETTINGS_INITIAL_WINDOW_SIZE`, it does
not apply the setting immediately but postpones it until data is added
to the write queue. If the new `SETTINGS_INITIAL_WINDOW_SIZE` is
applied while responding to the client, and a stream has been marked as
blocked (`stream->xmit.is_blocked = 1`) due to an exhausted HTTP/2
flow-control window while it still has pending HEADERS frames to send,
a kernel panic occurs. When applying SETTINGS_INITIAL_WINDOW_SIZE, we
update the window size of blocked streams. However, the stream marked
as `is_blocked` are not yet in the blocked streams queue because
transmission of the stream has not completed. See stack trace for
details.
To fix this, settings are now applied immediately upon receipt, and a
new frame-sending function `__tfw_h2_send_frame_local()` is introduced.
Unlike the existing implementation, this function bypasses the work
queue and queues the new frame directly into the connection's (not
socket's) write queue, or into the stream's postponed queue if a HEADERS
block is currently being transmitted.

This approach has several advantages:
1. It avoids unnecessary use of the work queue, since the code already
   runs under the lock and the frame is prepared on the local CPU.

2. It applies the new settings as early as possible, prioritizing
   transmission of the SETTINGS ACK before DATA frames. This behavior
   is consistent with RFC 9113 Section 6.5.3, which states:
   "A recipient of a SETTINGS frame in which the ACK flag is not set
   MUST apply the updated settings as soon as possible upon receipt
   ...
   Once all values have been processed, the recipient MUST immediately
   emit a SETTINGS frame with the ACK flag set."
3. It enables future optimizations, such as appending new frames to an
   existing skb in the queue instead of allocating a new one.

part of stack trace:
tfw_h2_stream_sched_insert_active+0xd5/0xe0
tfw_h2_sched_activate_stream+0x69/0x80 [tempesta_fw]
tfw_h2_apply_new_settings+0x1f3/0x260 [tempesta_fw]
ss_skb_tcp_entail+0x43/0x3c0 [tempesta_fw]
ss_skb_tcp_entail_list+0x124/0x560 [tempesta_fw]
tfw_h2_stream_send_postponed+0x28/0xc0 [tempesta_fw]
tfw_h2_make_frames+0x604/0xac0 [tempesta_fw]
tfw_connection_fill_sk_write_queue+0x119/0x260 [tempesta_fw]
tcp_push_pending_frames+0x33/0x160

The second issue is minor but still fixed by this patch. When two or
more SETTINGS frames are received with the same parameter but different
values, each value must be applied immediately, and a SETTINGS ACK must
be sent for each update. Before this fix, only the last value was
applied, overriding previous ones and causing some updates to be lost.

For example, if Tempesta receives SETTINGS_HEADER_TABLE_SIZE=0 and then
SETTINGS_HEADER_TABLE_SIZE=4096, it must apply both values. The sender
may intend to clear the table before applying the new settings, sending
0 table size. Previously, Tempesta applied only the last value (4096),
ignoring previous update.
`tfw_hpack_enc_tbl_write_sz()` is a part of framing, it is not a part of
hpack implementation
@const-t const-t force-pushed the kt-fix-hpack-desync branch from b462bbf to 8c5bd58 Compare June 13, 2026 09:39
@const-t const-t linked an issue Jun 25, 2026 that may be closed by this pull request
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Kernel panic in stream scheduler Corrupted content error

2 participants